[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.
On 8/8/2016 11:40 PM, Jan Beulich wrote:
On 12.07.16 at 11:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
@@ -178,8 +179,34 @@ static int hvmemul_do_io(
break;
case X86EMUL_UNHANDLEABLE:
{
- struct hvm_ioreq_server *s =
- hvm_select_ioreq_server(curr->domain, &p);
+ struct hvm_ioreq_server *s;
+
+ if ( is_mmio )
+ {
+ unsigned long gmfn = paddr_to_pfn(addr);
+ p2m_type_t p2mt;
+
+ (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
+
+ if ( p2mt == p2m_ioreq_server )
+ {
+ unsigned int flags;
+
+ if ( dir != IOREQ_WRITE )
+ s = NULL;
+ else
+ {
+ s = p2m_get_ioreq_server(currd, &flags);
+
+ if ( !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
+ s = NULL;
+ }
+ }
+ else
+ s = hvm_select_ioreq_server(currd, &p);
+ }
+ else
+ s = hvm_select_ioreq_server(currd, &p);
Wouldn't it both be more natural and make the logic even easier
to follow if s got set to NULL up front, all the "else"-s dropped,
and a simple
if ( !s )
s = hvm_select_ioreq_server(currd, &p);
be done in the end?
Thanks, Jan. I'll have a try.
@@ -5447,6 +5452,21 @@ static int hvmop_set_mem_type(
if ( !is_hvm_domain(d) )
goto out;
+ if ( a.hvmmem_type == HVMMEM_ioreq_server )
+ {
+ unsigned int flags;
+ struct hvm_ioreq_server *s;
+
+ /* HVMMEM_ioreq_server is only supported for HAP enabled hvm. */
+ if ( !hap_enabled(d) )
+ goto out;
+
+ /* Do not change to HVMMEM_ioreq_server if no ioreq server mapped. */
+ s = p2m_get_ioreq_server(d, &flags);
+ if ( s == NULL )
+ goto out;
Either drop s as an intermediate variable altogether (preferred), or
constify it properly.
Got it.
+int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
+ uint32_t type, uint32_t flags)
+{
+ struct hvm_ioreq_server *s;
+ int rc;
+
+ /* For now, only HVMMEM_ioreq_server is supported. */
+ if ( type != HVMMEM_ioreq_server )
+ return -EINVAL;
+
+ /* For now, only write emulation is supported. */
+ if ( flags & ~(XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
+ return -EINVAL;
+
+ spin_lock(&d->arch.hvm_domain.ioreq_server.lock);
This lock did get converted to a recursive one a little while back.
Oh. I see, should use the spin_lock_recursive/spin_unlock_recursive
pair. Thanks.
+ rc = -ENOENT;
+ list_for_each_entry ( s,
+ &d->arch.hvm_domain.ioreq_server.list,
+ list_entry )
+ {
+ if ( s == d->arch.hvm_domain.default_ioreq_server )
+ continue;
+
+ if ( s->id == id )
+ {
+ rc = p2m_set_ioreq_server(d, flags, s);
+ if ( rc == 0 )
+ dprintk(XENLOG_DEBUG, "%u %s type HVMMEM_ioreq_server.\n",
+ s->id, (flags != 0) ? "mapped to" : "unmapped from");
Is this really useful?
Sorry, are you referring to this debug message?
I believe it helps, especially when multiple ioreq servers are
claiming/disclaiming their ownership of
the HVMMEM_ioreq_server. :)
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -132,6 +132,13 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m,
ept_entry_t *entry,
entry->r = entry->w = entry->x = 1;
entry->a = entry->d = !!cpu_has_vmx_ept_ad;
break;
+ case p2m_ioreq_server:
+ entry->r = 1;
+ entry->w = !(p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS);
+ entry->x = 0;
+ entry->a = !!cpu_has_vmx_ept_ad;
+ entry->d = entry->w && cpu_has_vmx_ept_ad;
For self-consistency, could this become
entry->d = entry->w && entry->a;
?
Yep. Thanks. :)
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3225,8 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
}
/* Need to hand off device-model MMIO to the device model */
- if ( p2mt == p2m_mmio_dm
- || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
+ if ( p2mt == p2m_mmio_dm )
Could you remind me again what the code being removed here gets
replaced by, or why it doesn't need any replacement?
HVMMEM_ioreq_server is now only supported for HAP enabled hvm, so
shadow code
does not need to handle this p2m type.
@@ -336,6 +336,23 @@ struct p2m_domain {
struct ept_data ept;
/* NPT-equivalent structure could be added here. */
};
+
+ struct {
+ spinlock_t lock;
+ /*
+ * ioreq server who's responsible for the emulation of
+ * gfns with specific p2m type(for now, p2m_ioreq_server).
+ */
+ struct hvm_ioreq_server *server;
+ /*
+ * flags specifies whether read, write or both operations
+ * are to be emulated by an ioreq server.
+ */
+ unsigned int flags;
+
+#define P2M_IOREQ_HANDLE_WRITE_ACCESS XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE
+#define P2M_IOREQ_HANDLE_READ_ACCESS XEN_HVMOP_IOREQ_MEM_ACCESS_READ
I think I did say so on a previous iteration already: I can't see the
value of these two defines, or in fact I can see these being actively
dangerous: The rest of your patch assume that each pair shares
their values (as there's no translation between them, but also no
BUILD_BUG_ON() ensuring they're identical).
Oh. I thought it was a coding style issue we were discussing -
previously, there was a
#define using the <underscore><uppercase> pattern, which should be
deprecated.
For the P2M_IOREQ_HANDLE_WRITE/READ_ACCESS definition, I agree they can
be removed.
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -89,7 +89,9 @@ typedef enum {
HVMMEM_unused, /* Placeholder; setting memory to this type
will fail for code after 4.7.0 */
#endif
- HVMMEM_ioreq_server
+ HVMMEM_ioreq_server /* Memory type claimed by an ioreq server; type
+ changes to this value are only allowed after
+ an ioreq server has claimed its ownership. */
Wouldn't it be worth also noting in the comment that only changes
to/from rw are permitted?
OK. Will add this comment in next version.
Yu
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
