Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-05 Thread Greg KH
On Sat, Jan 05, 2019 at 01:43:55AM -0800, Roman Kiryanov wrote:
> > Why isn't this in drivers/platform/goldfish/ ?
> 
> I was not sure where to put, but this driver is not a platform one, it
> is a pci one.

You are controlling a "platform" device, the goldfish platform.  What
makes this different from any of the other goldfish platform drivers in
that directory other than you are triggering off of a virtual pci
device?

thanks,

greg k-h


Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-05 Thread Roman Kiryanov
> Why isn't this in drivers/platform/goldfish/ ?

I was not sure where to put, but this driver is not a platform one, it
is a pci one.


Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-05 Thread Roman Kiryanov
Hi. Thank you for looking into my patch.

> If they can not, then just remove the
> WARN_ON check as it is not needed at all.

No, these things don't happen. I will remove these lines.


Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-05 Thread Greg KH
On Thu, Jan 03, 2019 at 06:13:11PM -0800, r...@google.com wrote:
> +static int as_release(struct inode *inode, struct file *filp)
> +{
> + struct as_allocated_blocks *allocated_blocks = filp->private_data;
> + struct as_device_state *state;
> + int blocks_size;
> + int i;
> +
> + WARN_ON(!allocated_blocks);
> + WARN_ON(!allocated_blocks->state);
> + WARN_ON(!allocated_blocks->blocks);
> + WARN_ON(allocated_blocks->blocks_size < 0);
> +
> + state = allocated_blocks->state;
> + blocks_size = allocated_blocks->blocks_size;
> +
> + if (mutex_lock_interruptible(>registers_lock))

I just took this chunk of code as an example of what you do all over
this file.  Please do not use WARN_ON() as a lazy way of saying "I have
no idea how to handle this random error that might happen, so I'm going
to punt to the user and crash the machine."

If these things really can happen, then properly check for them and
handle the error correctly.  If they can not, then just remove the
WARN_ON check as it is not needed at all.

As it is, this code is obviously broken because if
allocated_blocks->state is NULL, you just crashed on the line after the
check.  So even if you did somehow want to "warn" for something like
this happening, you did not handle it and killed the machine.

All of the WARN_ON can be removed here as I bet you are testing for
things that can never happen.  And if it could happen, then properyl
test for it.

as-is, this code is not ok at all.

Also, along these lines, who else is reviewing this code before you send
it out?  Surely you are not reyling on just me to do that, you are
taking advantage of the huge numbers of reviewers inside your company
that could have told you this before posting it, right?

Please do so.

greg k-h


Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-05 Thread Greg KH
On Thu, Jan 03, 2019 at 06:13:11PM -0800, r...@google.com wrote:
> From: Roman Kiryanov 
> 
> A driver for the Goldfish Android emulator that occupies
> address space to use it with the memory sharing device
> on the QEMU side. The memory sharding device allocates
> subranges and populate them with actual RAM.
> This allows sharing host's memory with the guest.

Why isn't this in drivers/platform/goldfish/ ?


[PATCH] drivers: misc: goldfish_address_space: add a driver

2019-01-03 Thread rkir
From: Roman Kiryanov 

A driver for the Goldfish Android emulator that occupies
address space to use it with the memory sharing device
on the QEMU side. The memory sharding device allocates
subranges and populate them with actual RAM.
This allows sharing host's memory with the guest.

Signed-off-by: Roman Kiryanov 
---
 drivers/misc/Kconfig  |   9 +
 drivers/misc/Makefile |   1 +
 drivers/misc/goldfish_address_space.c | 742 ++
 .../linux/goldfish/goldfish_address_space.h   |  27 +
 4 files changed, 779 insertions(+)
 create mode 100644 drivers/misc/goldfish_address_space.c
 create mode 100644 include/uapi/linux/goldfish/goldfish_address_space.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f417b06e11c5..66518bbe4217 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -521,6 +521,15 @@ config PVPANIC
  a paravirtualized device provided by QEMU; it lets a virtual machine
  (guest) communicate panic events to the host.
 
+config GOLDFISH_ADDRESS_SPACE
+   tristate "A Goldfish driver that talks to the memory sharing device in 
QEMU"
+   depends on PCI
+   depends on GOLDFISH
+   help
+ A Goldfish driver that allocates address space ranges in the guest to
+ populate them later in the host. This allows sharing host's memory
+ with the guest.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e39ccbbc1b3a..11c9f9d7d3a1 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_PCI_ENDPOINT_TEST)   += pci_endpoint_test.o
 obj-$(CONFIG_OCXL) += ocxl/
 obj-y  += cardreader/
 obj-$(CONFIG_PVPANIC)  += pvpanic.o
+obj-$(CONFIG_GOLDFISH_ADDRESS_SPACE) += goldfish_address_space.o
diff --git a/drivers/misc/goldfish_address_space.c 
b/drivers/misc/goldfish_address_space.c
new file mode 100644
index ..3605adcdaf2e
--- /dev/null
+++ b/drivers/misc/goldfish_address_space.c
@@ -0,0 +1,742 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+MODULE_DESCRIPTION("A driver for the Goldfish Android emulator that occupies "
+  "address space to use it with the memory sharing device "
+  "on the QEMU side. The memory sharding device allocates "
+  "subranges and populate them with actual RAM. "
+  "This allows sharing host's memory with the guest.");
+MODULE_AUTHOR("Roman Kiryanov ");
+MODULE_LICENSE("GPL v2");
+
+enum as_register_id {
+   AS_REGISTER_COMMAND = 0,
+   AS_REGISTER_STATUS = 4,
+   AS_REGISTER_GUEST_PAGE_SIZE = 8,
+   AS_REGISTER_BLOCK_SIZE_LOW = 12,
+   AS_REGISTER_BLOCK_SIZE_HIGH = 16,
+   AS_REGISTER_BLOCK_OFFSET_LOW = 20,
+   AS_REGISTER_BLOCK_OFFSET_HIGH = 24,
+};
+
+enum as_command_id {
+   AS_COMMAND_ALLOCATE_BLOCK = 1,
+   AS_COMMAND_DEALLOCATE_BLOCK = 2,
+};
+
+#define AS_PCI_VENDOR_ID   0x607D
+#define AS_PCI_DEVICE_ID   0xF153
+#define AS_MAGIC_U32   (AS_PCI_VENDOR_ID << 16 | AS_PCI_DEVICE_ID)
+#define AS_ALLOCATED_BLOCKS_INITIAL_CAPACITY 32
+
+enum as_pci_bar_id {
+   AS_PCI_CONTROL_BAR_ID = 0,
+   AS_PCI_AREA_BAR_ID = 1,
+};
+
+struct as_driver_state;
+
+struct as_device_state {
+   u32 magic;
+
+   struct miscdevice   miscdevice;
+   struct pci_dev  *dev;
+   struct as_driver_state  *driver_state;
+
+   void __iomem*io_registers;
+
+   void*address_area;  /* to claim the address space */
+
+   /* physical address to allocate from */
+   unsigned long   address_area_phys_address;
+
+   struct mutexregisters_lock; /* protects registers */
+
+   wait_queue_head_t   wake_queue; /* to wait for the hardware */
+
+   int hw_done;/* to say hw is done */
+};
+
+struct as_driver_state {
+   struct as_device_state *device_state;
+   struct pci_driver pci;
+};
+
+struct as_block {
+   u64 offset;
+   u64 size;
+};
+
+struct as_allocated_blocks {
+   struct as_device_state *state;
+
+   struct as_block *blocks;  /* a dynamic array of allocated blocks */
+   int blocks_size;
+   int blocks_capacity;
+   struct mutex blocks_lock; /* protects operations with blocks */
+};
+
+static void __iomem *as_register_address(void __iomem *base,
+int offset)
+{
+   return ((char __iomem *)base) + offset;
+}
+
+static void as_write_register(void __iomem *registers,
+ int offset,
+ u32 value)
+{
+   writel(value,