Re: [Xen-devel] [PATCH v4 5/8] ioreq-server: add support for multiple servers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 April 2014 13:43
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v4 5/8] ioreq-server: add support for multiple servers
> 
> >>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> > Secondary servers use guest pages to communicate with emulators, in
> > the same way as the default server. These pages need to be in the
> > guest physmap otherwise there is no suitable reference that can be
> > queried by an emulator in order to map them. Therefore a pool of
> > pages in the current E820 reserved region, just below the special
> > pages is used. Secondary servers allocate from and free to this pool
> > as they are created and destroyed.
> 
> Ah, here is the answer to the question I raised on patch 6 - somehow
> I managed to look at them in wrong order. Nevertheless, and also in
> the context of the discussion we had with Stefano yesterday, we may
> want/need to think of a way to allow pages to be trackable without
> being mapped in the physmap.
> 

Possibly. That would be rather a large amount of scope-creep to these patches 
though.

> > @@ -60,11 +76,27 @@ struct hvm_ioreq_server {
> >      /* Lock to serialize access to buffered ioreq ring */
> >      spinlock_t             bufioreq_lock;
> >      evtchn_port_t          bufioreq_evtchn;
> > +    struct list_head       mmio_range_list;
> > +    struct list_head       portio_range_list;
> > +    struct list_head       pcidev_list;
> 
> Wouldn't these better be range sets? I realize this might conflict with
> the RCU manipulation of the entries, but perhaps the rangesets could
> get their interface extended if this is strictly a requirement (otoh hand
> I can't see why you couldn't get away with "normal" freeing, since
> changes to these lists shouldn't be frequent, and hence not be
> performance critical).
> 

The lists are accessed without lock by a vcpu requesting emulation so they need 
to be RCU. I'll look at the range set code and see if that is feasible.

> Also, I didn't see a limit being enforced on the number of elements
> that can be added to these lists, yet allowing this to be unlimited is
> a latent security issue.
> 

Guest domains cannot add to the lists, only the emulating domain, but if that 
is unprivileged then, yes, that is a security issue.

> >  struct hvm_domain {
> > +    /* Guest page range used for non-default ioreq servers */
> > +    unsigned long           ioreq_gmfn_base;
> > +    unsigned int            ioreq_gmfn_count;
> > +    unsigned long           ioreq_gmfn_mask;
> > +
> > +    /* Lock protects all other values in the following block */
> >      spinlock_t              ioreq_server_lock;
> > -    struct hvm_ioreq_server *ioreq_server;
> > +    ioservid_t              ioreq_server_id;
> > +    struct list_head        ioreq_server_list;
> > +    unsigned int            ioreq_server_count;
> > +    struct hvm_ioreq_server *default_ioreq_server;
> > +
> > +    /* Cached CF8 for guest PCI config cycles */
> > +    uint32_t                pci_cf8;
> > +    spinlock_t              pci_lock;
> 
> Please consider padding when adding new fields here - try grouping
> 64-bit quantities together rather than alternating between 32- and
> 64-bit ones.

Why do we need to care about padding? Re-ordering for efficiency of space is 
reasonable.

> 
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -228,7 +228,8 @@ int prepare_ring_for_helper(struct domain *d,
> unsigned long gmfn,
> >                              struct page_info **_page, void **_va);
> >  void destroy_ring_for_helper(void **_va, struct page_info *page);
> >
> > -bool_t hvm_send_assist_req(struct vcpu *v, const ioreq_t *p);
> > +bool_t hvm_send_assist_req(struct vcpu *v, ioreq_t *p);
> 
> Any reason you couldn't avoid adding the const in one of the earlier
> patches?
> 

You asked for it in a previous review. I'm happy to lose the const again.

> > +#define HVMOP_map_io_range_to_ioreq_server 19
> > +struct xen_hvm_map_io_range_to_ioreq_server {
> > +    domid_t domid;                  /* IN - domain to be serviced */
> > +    ioservid_t id;                  /* IN - handle from
> HVMOP_register_ioreq_server */
> > +    int is_mmio;                    /* IN - MMIO or port IO? */
> > +    uint64_aligned_t start, end;    /* IN - inclusive start and end of 
> > range */
> > +};
> > +typedef struct xen_hvm_map_io_range_to_ioreq_server
> xen_hvm_map_io_range_to_ioreq_server_t;
> >
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_map_io_range_to_ioreq_server_
> t);
> > +
> > +#define HVMOP_unmap_io_range_from_ioreq_server 20
> > +struct xen_hvm_unmap_io_range_from_ioreq_server {
> > +    domid_t domid;          /* IN - domain to be serviced */
> > +    ioservid_t id;          /* IN - handle from 
> > HVMOP_register_ioreq_server */
> > +    uint8_t is_mmio;        /* IN - MMIO or port IO? */
> 
> Please use uint8_t above too, and move the field ahead of "id" for
> better packing. I'm also not sure the "is_" prefix is really useful here.

Ok.

> And for this to be usable with other architectures that may have
> address spaces other than memory and I/O ports it would seem
> desirable to not consider this a boolean, but an enumerator.

Maybe it would be better to consolidate io ranges and pci devs then and the 
existing ioreq type values in the interface. I.e:

#define IOREQ_TYPE_PIO          0 /* pio */
#define IOREQ_TYPE_COPY         1 /* mmio ops */
#define IOREQ_TYPE_PCI_CONFIG   2 /* pci config ops */

>  In the
> end a third address space could immediately be PCI space, thus
> eliminating the need for the two special ops below. I.e. this could
> follow more closely ACPI's address space handling - there's nothing
> inherently wrong with an MSR based I/O interface for example.
> 

Ah, I see you had the same thought :-)

> > +#define HVMOP_map_pcidev_to_ioreq_server 21
> > +struct xen_hvm_map_pcidev_to_ioreq_server {
> > +    domid_t domid;      /* IN - domain to be serviced */
> > +    ioservid_t id;      /* IN - handle from HVMOP_register_ioreq_server */
> > +    uint16_t bdf;       /* IN - PCI bus/dev/func */
> > +};
> > +typedef struct xen_hvm_map_pcidev_to_ioreq_server
> xen_hvm_map_pcidev_to_ioreq_server_t;
> >
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_map_pcidev_to_ioreq_server_t);
> > +
> > +#define HVMOP_unmap_pcidev_from_ioreq_server 22
> > +struct xen_hvm_unmap_pcidev_from_ioreq_server {
> > +    domid_t domid;      /* IN - domain to be serviced */
> > +    ioservid_t id;      /* IN - handle from HVMOP_register_ioreq_server */
> > +    uint16_t bdf;       /* IN - PCI bus/dev/func */
> 
> Both of these need a PCI segment/domain added. 

Ok.

> Also what's the
> point of having two identical structures of map and unmap?
> 

Good point. I'll collapse them down.

  Paul

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel