Re: [PATCH v2 12/30] s390/pci: get SHM information from list pci

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 1/27/22 8:41 AM, Pierre Morel wrote:


On 1/26/22 11:13, Claudio Imbrenda wrote:
On Tue, 18 Jan 2022 11:36:06 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

On 1/14/22 21:31, Matthew Rosato wrote:
KVM will need information on the special handle mask used to indicate
emulated devices.  In order to obtain this, a new type of list pci call
must be made to gather the information.  Extend clp_list_pci_req to
also fetch the model-dependent-data field that holds this mask.

Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
---
   arch/s390/include/asm/pci.h     |  1 +
   arch/s390/include/asm/pci_clp.h |  2 +-
   arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
   3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 00a2c24d6d2b..f3cd2da8128c 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
   int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
   int clp_get_state(u32 fid, enum zpci_state *state);
   int clp_refresh_fh(u32 fid, u32 *fh);
+int zpci_get_mdd(u32 *mdd);
   /* UID */
   void update_uid_checking(bool new);
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 124fadfb74b9..d6bc324763f3 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -76,7 +76,7 @@ struct clp_req_list_pci {
   struct clp_rsp_list_pci {
       struct clp_rsp_hdr hdr;
       u64 resume_token;
-    u32 reserved2;
+    u32 mdd;
       u16 max_fn;
       u8            : 7;
       u8 uid_checking        : 1;
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index bc7446566cbc..308ffb93413f 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
   }
   static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
-                u64 *resume_token, int *nentries)
+                u64 *resume_token, int *nentries, u32 *mdd)
   {
       int rc;
@@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
       *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
           rrb->response.entry_size;
       *resume_token = rrb->response.resume_token;
+    if (mdd)
+        *mdd = rrb->response.mdd;
       return rc;
   }
@@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
       int nentries, i, rc;
       do {
-        rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+        rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
           if (rc)
               return rc;
           for (i = 0; i < nentries; i++)
@@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
       int nentries, i, rc;
       do {
-        rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+        rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
           if (rc)
               return rc;
           fh_list = rrb->response.fh_list;
@@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
       return rc;
   }
+int zpci_get_mdd(u32 *mdd)
+{
+    struct clp_req_rsp_list_pci *rrb;
+    u64 resume_token = 0;
+    int nentries, rc;
+
+    if (!mdd)
+        return -EINVAL;

I think this tests is not useful.
The caller must take care not to call with a NULL pointer,
what the only caller today make sure.

what if the caller does it anyway?

I think the test is useful. if passing NULL is a bug, then maybe
consider using BUG_ON, or WARN_ONCE

I think generally the caller is responsible for the test.
In our case for example the caller can use directly the address
of a u32 allocated on the stack or globaly and he knows if a test is useful or not.

Of course we can systematically check in every kernel function all pointer parameters against NULL. But this is not userland, not even a inter-architecture core function and we can expect the kernel programmer to programm correctly.

I appreciate your optimism :)


For our special case zpci_get_mdd() nor clp_list_pci_req() do access *mdd if mdd is NULL so that giving a NULL mdd pointer will not trigger a fault. Also, the function is named zpci_get_mdd(u32 *mdd) if the caller do not give a pointer to mdd which would be quite stupid to call a function zpci_get_mdd() in this circumstance he will just no get mdd, no side effect. So the only purpose having this test here is to say the caller that he forgot to check his mdd allocation.
My opinion is that he should have check.

Based on the thread of conversation, I'm going to assume you meant 'My opinion that he should -not- have the check here'

So, I'm generally a fan of being a bit defensive in paths that are not performance-intensive and would therefore typically tend to agree with Claudio. But in this particular case, I'm willing to just drop this check and move on, primarily because 1) we only have a single caller already checking this case and 2) I don't really anticipate any additional callers later.

@Niklas do you care / is it OK to keep your R-b if I remove this if(!mdd) check?




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux