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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 December 2020 12:05
> To: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>;
> George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; 
> Julien Grall
> <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH V3 08/23] xen/ioreq: Move x86's ioreq_server to struct 
> domain
> 
> On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> 
> Applicable parts
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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?
> 

Yes, I think that is a stale part of the comment from the days of the default 
ioreq server. It can be dropped.

  Paul

> Jan