Re: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2020 11:06, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index d1cfc8fb4a..e82307bdae 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4591,6 +4591,26 @@ int xenmem_add_to_physmap_one(
>>      return rc;
>>  }
>>
>> +unsigned int arch_resource_max_frames(
>> +    struct domain *d, unsigned int type, unsigned int id)
>> +{
>> +    unsigned int nr = 0;
>> +
>> +    switch ( type )
>> +    {
>> +#ifdef CONFIG_HVM
>> +    case XENMEM_resource_ioreq_server:
>> +        if ( !is_hvm_domain(d) )
>> +            break;
>> +        /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. */
>> +        nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), 
>> PAGE_SIZE);
> The buf-ioreq ring is optional

Yes, but it's position within the resource, and effect on the position
of the ioreq page(s), is not.

>  so a caller using this value may still get a resource acquisition failure 
> unless the id is used to actually look up and check the ioreq server in 
> question for the actual maximum.

Yes, but that is potentially true of *any* acquisition attempt, even if
the id matches, because maybe someone else has destroyed the ioreq
server, or the domain, in the meantime.


What we have is an mmap() where the caller needs to know to not try and
map page 0 for an ioreq server where buf-ioreq doesn't exist.

This is a direct consequence of:

#define XENMEM_resource_ioreq_server_frame_bufioreq 0
#define XENMEM_resource_ioreq_server_frame_ioreq(n) (1 + (n))

and in practice, what a qemu/demu/other needs to do to map just the
ioreq frames (in a manner compatible with >127 vcpu HVM domains) is to
query the resource size and map size-1 pages from offset 1. 

~Andrew