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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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).

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.

>  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.

> --- 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?

> +#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.
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. 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.

> +#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. Also what's the
point of having two identical structures of map and unmap?

Jan

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