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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2026 15:59, Roger Pau Monné wrote:
> On Fri, Mar 13, 2026 at 04:35:03PM +0000, Thierry Escande wrote:
>> --- 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 ) )
>                                 ^ extra space, here and at the
>                                   closing parenthesis.
> 
> Line length is also past the 80 character limit, same below in
> HVMOP_get_ecam_space.
> 
>> +            return -EFAULT;
> 
> This operation (and the matching get variant) needs an XSM check.
> 
>> +
>> +        d = rcu_lock_domain_by_any_id(ecam.domid);
>> +        if ( d == NULL )
>> +            return -ESRCH;
>> +
>> +        if ( d->arch.ecam_addr ) {
> 
> Coding style, opening braces should be on a new line.
> 
>> +            rcu_unlock_domain(d);
>> +            return -EFAULT;
> 
> This would better return -EBUSY

I agree, yet I'd like to suggest that this may want changing further: If
one can "set" the address, shouldn't one also be able to "clear" it? That
could (pretty) naturally be expressed by ecam.addr being 0 in the request.
Which would then require permitting non-0 .ecam_addr in that specific
case.

>> +        if ( (ecam.size >> 28) || (!ecam.addr) ) {
>                                      ^ the parenthesis here are
>                                      unneeded.
> 
>> +            rcu_unlock_domain(d);
>> +            return -EINVAL;
>> +        }
>> +
>> +        d->arch.ecam_addr = ecam.addr;
>> +        d->arch.ecam_size = ecam.size;
> 
> I'm a bit worried about a domain being able to set it's own ECAM hole,
> assessing all the side-effects of this might be complex.
> 
> Won't the code here better check the region passed in the hypercall is
> indeed not mapped in the p2m, so that trapping of ECAM accesses works
> as expected?
> 
> Also, how does the ECAM hole get setup on native?  I assume there are
> some magic registers in the PCI config space of a platform device that
> the firmware uses to position the ECAM space?

That may even be outside of any device's config space, e.g. custom MMIO.
I didn't check, but I guess that may also be the case for Q35.

Jan