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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

Jan