[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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. > @@ -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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |