Re: [PATCH V3 08/23] xen/ioreq: Move x86's ioreq_server to struct domain

[Oleksandr <olekstysh@gmail.com>]

On 07.12.20 14:04, Jan Beulich wrote:

Hi Jan

> On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> The IOREQ is a common feature now and this struct will be used
>> on Arm as is. Move it to common struct domain. This also
>> significantly reduces the layering violation in the common code
>> (*arch.hvm* usage).
>>
>> We don't move ioreq_gfn since it is not used in the common code
>> (the "legacy" mechanism is x86 specific).
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Applicable parts
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thank you.


> yet with a question, but maybe more to Paul than to you:
>
>> --- a/xen/include/asm-x86/hvm/domain.h
>> +++ b/xen/include/asm-x86/hvm/domain.h
>> @@ -63,8 +63,6 @@ struct hvm_pi_ops {
>>       void (*vcpu_block)(struct vcpu *);
>>   };
>>   
>> -#define MAX_NR_IOREQ_SERVERS 8
>> -
>>   struct hvm_domain {
>>       /* Guest page range used for non-default ioreq servers */
>>       struct {
>> @@ -73,12 +71,6 @@ struct hvm_domain {
>>           unsigned long legacy_mask; /* indexed by HVM param number */
>>       } ioreq_gfn;
>>   
>> -    /* Lock protects all other values in the sub-struct and the default */
>> -    struct {
>> -        spinlock_t              lock;
>> -        struct ioreq_server *server[MAX_NR_IOREQ_SERVERS];
>> -    } ioreq_server;
>> -
>>       /* Cached CF8 for guest PCI config cycles */
>>       uint32_t                pci_cf8;
>>   
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -316,6 +316,8 @@ struct sched_unit {
>>   
>>   struct evtchn_port_ops;
>>   
>> +#define MAX_NR_IOREQ_SERVERS 8
>> +
>>   struct domain
>>   {
>>       domid_t          domain_id;
>> @@ -523,6 +525,14 @@ struct domain
>>       /* Argo interdomain communication support */
>>       struct argo_domain *argo;
>>   #endif
>> +
>> +#ifdef CONFIG_IOREQ_SERVER
>> +    /* Lock protects all other values in the sub-struct and the default */
>> +    struct {
>> +        spinlock_t              lock;
>> +        struct ioreq_server     *server[MAX_NR_IOREQ_SERVERS];
>> +    } ioreq_server;
>> +#endif
> The comment gets merely moved, but what "default" does it talk about?
> Is this a stale part which would better be dropped at this occasion?

I saw Paul's answer, will drop stale part.


-- 
Regards,

Oleksandr Tyshchenko