Re: [PATCH 09/17] xev/hvm: Add HVMOP_get|set_ecam_space hypercalls

[Thierry Escande <thierry.escande@xxxxxxxxxx>]

On 5/4/26 17:12, Jan Beulich wrote:
> On 13.03.2026 17:35, Thierry Escande wrote:
>> This patch adds 2 HVMOP hypercalls, HVMOP_get|set_ecam_space, used to
>> set and get the base address and size of the PCIe ECAM space as
>> configured by hvmloader.
>>
>> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
> 
> Just in case we want to stick to these (see Roger's earlier comments
> throughout the series), a few remarks here:
> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5195,6 +5195,58 @@ long do_hvm_op(unsigned long op, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          rc = current->hcall_compat ? compat_altp2m_op(arg) : 
>> do_altp2m_op(arg);
>>          break;
>>  
>> +    case HVMOP_set_ecam_space: {
>> +        xen_hvm_ecam_space_t ecam;
>> +        struct domain *d;
>> +
>> +        if ( copy_from_guest( &ecam, guest_handle_cast(arg, 
>> xen_hvm_ecam_space_t), 1 ) )
>> +            return -EFAULT;
>> +
>> +        d = rcu_lock_domain_by_any_id(ecam.domid);
>> +        if ( d == NULL )
>> +            return -ESRCH;
>> +
>> +        if ( d->arch.ecam_addr ) {
>> +            rcu_unlock_domain(d);
>> +            return -EFAULT;
>> +        }
>> +
>> +        if ( (ecam.size >> 28) || (!ecam.addr) ) {
>> +            rcu_unlock_domain(d);
>> +            return -EINVAL;
>> +        }
>> +
>> +        d->arch.ecam_addr = ecam.addr;
>> +        d->arch.ecam_size = ecam.size;
> 
> Shorter (and easier to follow as well as less error prone as to the
> rcu_unlock_domain())
> 
>         if ( d->arch.ecam_addr )
>             rc = -E...;
>         else if ( (ecam.size >> 28) || !ecam.addr )
>             rc = -EINVAL;
>         else
>         {
>             d->arch.ecam_addr = ecam.addr;
>             d->arch.ecam_size = ecam.size;
>         }
> 
> all utilizing ...
> 
>> +        rcu_unlock_domain(d);
> 
> ... this.

Will rework that part.

> 
> The magic 28 also needs (a) explaining and/or (b) abstracting (a
> suitably named #define might address both).
> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -166,6 +166,17 @@ struct xen_hvm_get_mem_type {
>>  typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_type_t);
>>  
>> +#define HVMOP_set_ecam_space    16
>> +#define HVMOP_get_ecam_space    17
>> +struct xen_hvm_ecam_space {
>> +    domid_t  domid;
>> +    uint16_t pad[3]; /* align next field on 8-byte boundary */
> 
> The comment, as is, is wrong for 32-bit HVM guests: There ...
> 
>> +    uint64_t addr;
> 
> ... this is only 4-byte aligned, and hence the entire structure only
> has 4-byte alignment, and hence the padding also only guarantees 4-
> byte alignment.

Right. Roger proposal fixes it:
    domid_t  domid;
    uint16_t pad;
    uint32_t size
    uint64_t addr;

Regards,



--
Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech