On 22.09.20 18:52, Jan Beulich wrote:
Hi Jan
On 22.09.2020 17:05, Oleksandr wrote:
2. *arch.hvm.params*: Two functions that use it
(hvm_alloc_legacy_ioreq_gfn/hvm_free_legacy_ioreq_gfn) either go into
arch code completely or
specific macro is used in common code:
#define ioreq_get_params(d, i) ((d)->arch.hvm.params[i])
If Arm has the concept of params, then perhaps. But I didn't think
Arm does ...
I think it has in some degree, there is a handling of
HVMOP_set_param/HVMOP_get_param and
also there is a code to setup HVM_PARAM_CALLBACK_IRQ.
I would prefer macro than moving functions to arch code (which are
equal and should remain in sync).
Yes, if the rest of the code is identical, I agree it's better to
merely abstract away small pieces like this one.
ok
3. *arch.hvm.hvm_io*: We could also use the following:
#define ioreq_get_io_completion(v) ((v)->arch.hvm.hvm_io.io_completion)
#define ioreq_get_io_req(v) ((v)->arch.hvm.hvm_io.io_req)
This way struct hvm_vcpu_io won't be used in common code as well.
But if Arm needs similar field, why keep them in arch.hvm.hvm_io?
Yes, Arm needs the "some" fields, but not "all of them" as x86 has.
For example Arm needs only the following (at least in the context of
this series):
+struct hvm_vcpu_io {
+ /* I/O request in flight to device model. */
+ enum hvm_io_completion io_completion;
+ ioreq_t io_req;
+
+ /*
+ * HVM emulation:
+ * Linear address @mmio_gla maps to MMIO physical frame @mmio_gpfn.
+ * The latter is known to be an MMIO frame (not RAM).
+ * This translation is only valid for accesses as per @mmio_access.
+ */
+ struct npfec mmio_access;
+ unsigned long mmio_gla;
+ unsigned long mmio_gpfn;
+};
But for x86 the number of fields is quite bigger. If they were in same
way applicable for both archs (as what we have with ioreq_server struct)
I would move it to the common domain. I didn't think of a better idea
than just abstracting accesses to these (used in common ioreq.c) two
fields by macro.
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -194,7 +194,7 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu
*sv, ioreq_t *p)
bool handle_hvm_io_completion(struct vcpu *v)
{
struct domain *d = v->domain;
- struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
+ ioreq_t io_req = ioreq_get_io_req(v);
struct hvm_ioreq_server *s;
struct hvm_ioreq_vcpu *sv;
enum hvm_io_completion io_completion;
@@ -209,14 +209,14 @@ bool handle_hvm_io_completion(struct vcpu *v)
if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
return false;
- vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
+ io_req.state = hvm_ioreq_needs_completion(&io_req) ?
STATE_IORESP_READY : STATE_IOREQ_NONE;
This is unlikely to be correct - you're now updating an on-stack
copy of the ioreq_t instead of what vio points at.
Oh, thank you for pointing this, I should have used ioreq_t *io_req =
&ioreq_get_io_req(v);
I don't like ioreq_get_io_req much), probably ioreq_req would sound a
little bit better?
msix_write_completion(v);
vcpu_end_shutdown_deferral(v);
- io_completion = vio->io_completion;
- vio->io_completion = HVMIO_no_completion;
+ io_completion = ioreq_get_io_completion(v);
+ ioreq_get_io_completion(v) = HVMIO_no_completion;
I think it's at least odd to have an lvalue with this kind of a
name. Perhaps want to drop "get" if it's really meant to be used
like this.
ok
@@ -247,7 +247,7 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct
hvm_ioreq_server *s)
for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
{
if ( !test_and_clear_bit(i, &d->ioreq_gfn.legacy_mask) )
- return _gfn(d->arch.hvm.params[i]);
+ return _gfn(ioreq_get_params(d, i));
}
return INVALID_GFN;
@@ -279,7 +279,7 @@ static bool hvm_free_legacy_ioreq_gfn(struct
hvm_ioreq_server *s,
for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
{
- if ( gfn_eq(gfn, _gfn(d->arch.hvm.params[i])) )
+ if ( gfn_eq(gfn, _gfn(ioreq_get_params(d, i))) )
break;
}
if ( i > HVM_PARAM_BUFIOREQ_PFN )
And these two are needed by Arm? Shouldn't Arm exclusively use
the new model, via acquire_resource?
I dropped HVMOP plumbing on Arm as it was requested. Only acquire
interface should be used.
This code is not supposed to be called on Arm, but it is a part of
common code and we need to find a way how to abstract away *arch.hvm.params*
Am I correct?
--
Regards,
Oleksandr Tyshchenko
